Skip to content

fix(security): do not HTML-purify Turnstile tokens - #29

Open
t0ma5 wants to merge 3 commits into
mindstellar:developfrom
t0ma5:fix/turnstile-raw-token
Open

fix(security): do not HTML-purify Turnstile tokens#29
t0ma5 wants to merge 3 commits into
mindstellar:developfrom
t0ma5:fix/turnstile-raw-token

Conversation

@t0ma5

@t0ma5 t0ma5 commented Aug 16, 2026

Copy link
Copy Markdown

Summary

  • Cloudflare Turnstile is optional in Shopclass (Settings → Spam and bots: auto / recaptcha / turnstile / none). When it is on, osc_check_captcha() sent cf-turnstile-response through Params::getParam(), which HTML-purifies the value.
  • Captcha tokens are opaque, not HTML. Purifying them can empty or alter the string so siteverify always fails. The same path applied to g-recaptcha-response.
  • Tokens are now read raw from POST only (osc_posted_captcha_token()). A query-string copy is ignored. Length cap for Turnstile is unchanged (2048).

Test plan

  • With Turnstile keys set, submit admin login / web login / register / recover and confirm siteverify succeeds
  • php tests/captcha-posted-token.php (POST kept as-is, GET ignored, HTMLPurifier contrast)
  • Confirm a ?cf-turnstile-response= query string is not accepted as a token

NOTE: I have been running a heavily customized version of Osclass for 12 years volunteersbase.com - Thank you for keeping the project alive! Here is my humble contribution, hope you can find it useful :)

Captcha tokens are opaque POST strings. Running them through HTMLPurifier can empty or alter the value so siteverify always fails.

Co-authored-by: Cursor <cursoragent@cursor.com>
@navjottomer

Copy link
Copy Markdown
Member

Thanks for this — the diagnosis is right and the test is a good addition.

I confirmed the bug independently: passing a token through the purifier turns & into &amp;, and a < truncates it, so a token containing either fails validation. Plain base64url tokens come through untouched, which is presumably why this went unnoticed for so long. Your test passes here too (7/7).

One request before merge, on the shape rather than the substance.

Core already has an idiom for reading a value without any transformation: Params::getParamString($name, false, false). That is what the installer uses for passwords, which have the same "must arrive byte-for-byte" requirement — see install-functions.php lines 371, 393 and 512. osc_posted_captcha_token() adds a new permanent public global that reads $_POST directly and bypasses Params entirely, and once a global helper ships we are committed to keeping it for plugin compatibility.

The one thing your helper genuinely adds over the existing idiom is POST-only enforcement, and that is worth keeping. Could it become a small private method on the captcha class, or a Params call plus an explicit request-method check at the call site, rather than a new global?

Separately, and not something you introduced — the guard immediately above is broken:

if ($gReCaptchaResponse !== '' || $gReCaptchaResponse !== false || $gReCaptchaResponse !== 0)

Those are ORed, so the condition is always true regardless of the value (php -r returns bool(true) for an empty string). The empty-token check has never actually fired. Since you are already in this function, folding that into the same PR would be welcome — but happy for it to be a follow-up if you would rather keep this one focused.

Also note the branch is currently conflicting with develop.

Drop the extra global helper. Use Params::getParamString(name, false, false) plus a POST method check, and fix the empty-token guard that was always true.
@t0ma5

t0ma5 commented Aug 17, 2026

Copy link
Copy Markdown
Author

Thanks — I took the shape change.

The extra global is gone. Call sites now use Params::getParamString($name, false, false) (same as installer passwords) plus an explicit POST method check. The always-true empty-token guard is === '' and returns false. Tests pin the unpurified Params path and that GET is rejected.

Still needs a merge of current develop for the CHANGELOG conflict; I can do that on the next push if you want it from this side.

@t0ma5
t0ma5 requested a review from navjottomer as a code owner August 17, 2026 15:00
…w-token

Keep the Turnstile token-read fix in the 6.2.0 Fixed section with the develop changelog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants